Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/public visors advertising #743

Merged
merged 48 commits into from
May 17, 2021
Merged

Feature/public visors advertising #743

merged 48 commits into from
May 17, 2021

Conversation

i-hate-nicknames
Copy link
Contributor

@i-hate-nicknames i-hate-nicknames commented Apr 27, 2021 •
edited
Loading

Did you run make format && make check?

Fixes #574

Changes:

On start visor registers itself in the service discovery. Visor attaches IPs of its local active network interface to the request;
If public_autoconnect field is set to true in config, visor will try to connect to 5 known visors on its start.

taras-skycoin and others added 30 commits November 18, 2020 15:42
@taras-skycoin
Advertise public visors.
d6f9fa9
@taras-skycoin
Fix linter
0f2dfe8
@taras-skycoin
Add explicit return.
c1f2a73
@taras-skycoin
Remove public_address field;Fix linter.
c3ecaf2
@taras-skycoin
Add ReadOnly address resolver client.
9b5cb14
@taras-skycoin
Add boolean field wich indicates if visor is on public address.
1ca9025
@taras-skycoin
Pass STCPR port to service-discover port.
741a4b4
@taras-skycoin
Merge remote-tracking branch 'upstream/develop' into advertise_public…
2a1cc24 
…_visors
@taras-skycoin
Fix sleep delays.
17d8596
@Darkren
Merge branch 'develop' of github.com:skycoin/skywire into advertise_p…
d840013 
…ublic_visors
@Darkren
Start merging old trusted visors with the new public visor concept
4426259
@Darkren
Add func to fetch default network interface name for OSX
9eba538
@Darkren
Include local visor IPs in the update request for visor service
ce12e5b
@Darkren
Clean up the code
00fc277
@Darkren
Fix import cycle, fix visor test
6568a24
@Darkren
Merge branch 'develop' of github.com:skycoin/skywire into feature/pub…
0ed9947 
…lic-visors-advertising
@Darkren
Merge branch 'develop' of github.com:skycoin/skywire into feature/pub…
c9ab872 
…lic-visors-advertising
@Darkren
Add debug logs
9d74cd9
@Darkren
Update deps
6ffe9d3
@Darkren
Add debug logs
fb05117
@Darkren
Make service updater stop retrying if visor is unreachable
a90a855
@Darkren
Properly stop ticker on updater exit
4cc790f
@Darkren
Remove debug logs
083cf99
@i-hate-nicknames
Merge branch 'develop' into feature/public-visors-advertising
c25e632
@i-hate-nicknames
Run make format
0f249ce
@i-hate-nicknames
Use errors.Is instead of string matching
dbc25f6
@i-hate-nicknames
Add exponential backoff to service update entry
6af273c
@i-hate-nicknames
Refactor entry update loop
d7b23ae
@i-hate-nicknames
Move POST service discovery to a separate func
0ebc644
@i-hate-nicknames
Add DefaultNetworkInterfaceIPs netutil function
10b8760
@i-hate-nicknames
Copy link
Contributor Author

One thing I dislike is that I had to rely on callbacks in autoconnect. This is due to the reason that autoconnect needs transport manager, but importing transport manager introduces dependency cycle: transport package depends on servicedisc package.

I'm thinking about restructuring it a better way, maybe splitting some packages. Input on this subject is welcome.

@ersonp
Copy link
Contributor

ersonp commented May 6, 2021

@i-hate-nicknames I was testing out the PR and skywire-cli visor gen-config generates config for v1.1.0 and not for v1.1.1.

@i-hate-nicknames
Copy link
Contributor Author

i-hate-nicknames commented May 6, 2021 •
edited
Loading

Weird, I just checked and it generated 1.1.1 for me. Is it possible that you didn't build with make build and accidentally ran an old binary?

@ersonp
Copy link
Contributor

ersonp commented May 6, 2021 •
edited
Loading

make build; make install solved it. And connect_to_public_visors mentioned above. I could not find it in the config. Perhaps you meant public_autoconnect.

jdknives
jdknives reviewed May 7, 2021
View reviewed changes
Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good work although some of the implementation details might be improvable.

pkg/servicedisc/client.go Outdated Show resolved Hide resolved
pkg/servicedisc/autoconnect.go Outdated Show resolved Hide resolved
pkg/servicedisc/autoconnect.go Outdated Show resolved Hide resolved
pkg/servicedisc/autoconnect.go Outdated Show resolved Hide resolved
pkg/servicedisc/autoconnect.go Outdated Show resolved Hide resolved
pkg/servicedisc/types.go Outdated Show resolved Hide resolved
pkg/visor/init.go Outdated Show resolved Hide resolved
alexadhy
alexadhy reviewed May 10, 2021
View reviewed changes
pkg/servicedisc/client.go Outdated Show resolved Hide resolved
jdknives
jdknives reviewed May 16, 2021
View reviewed changes
pkg/servicedisc/autoconnect.go Outdated Show resolved Hide resolved
pkg/servicedisc/autoconnect.go Outdated Show resolved Hide resolved
pkg/servicedisc/autoconnect.go Outdated Show resolved Hide resolved
alexadhy
alexadhy approved these changes May 17, 2021
View reviewed changes
Copy link
Contributor

@alexadhy alexadhy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome code, lgtm.

@jdknives jdknives merged commit 944474c into skycoin:develop May 17, 2021
@jdknives jdknives mentioned this pull request Jul 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexadhy alexadhy alexadhy approved these changes

@jdknives jdknives jdknives approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@i-hate-nicknames @ersonp @jdknives @alexadhy @taras-skycoin @Darkren